Fix default auditing options. #60739

tallclair · 2018-03-02T23:23:03Z

Log backend defaults to blocking mode (backwards compatability)
Webhook backend defaults to throttled
Fix webhook validation
Add options test

Which issue(s) this PR fixes:
Fixes #60719

Special notes for your reviewer:
This PR is an alternative fix to #60727. If the rollback goes in first, I'll rebase this on a roll-forward.

Release note:
-->

NONE

CaoShuFeng · 2018-03-05T07:27:33Z

staging/src/k8s.io/apiserver/pkg/server/options/audit.go

+		// Don't validate the unused options.
+		return nil
+	}
+	config := options.BatchConfig


What will happen if --audit-log-batch-max-wait is set to zero?

It will hotloop. Not great, but not exactly invalid. I'd prefer to leave it as-is here.

thockin · 2018-03-05T07:27:38Z

This change is

crassirostris

Overall LGTM

Log backend defaults to blocking mode (backwards compatability)

This is still a rollback. I think batching behavior is better by default

crassirostris · 2018-03-05T09:50:05Z

staging/src/k8s.io/apiserver/plugin/pkg/audit/webhook/webhook.go

@@ -101,3 +101,7 @@ func (b *backend) processEvents(ev ...*auditinternal.Event) error {
 		return b.w.RestClient.Post().Body(&list).Do()
 	}).Error()
 }
+
+func (b *backend) String() string {


Why not Name()?

This way just supports calling fmt.Sprintf("%s", ...), and resolves to something if String isn't set. I opted not to add it to the backend interface. But I don't have a strong preference, it's sort of a hack to get the test working.

CaoShuFeng · 2018-03-05T09:55:41Z

I think batching behavior is better by default

I am OK with it, if the new behavior is well documented.

tallclair · 2018-03-05T18:15:11Z

Re: default to batch - I think we should merge this (revert defaults), and then follow up with a separate PR to change just the log default (maybe wait for 1.11). I could add a release note or help text stating that the default will change in the future.

sttts · 2018-03-06T08:27:47Z

staging/src/k8s.io/apiserver/pkg/audit/union.go

+func (u union) String() string {
+	var backendStrings []string
+	for _, backend := range u.backends {
+		backendStrings = append(backendStrings, fmt.Sprintf("%s", backend))


nit: why not backend.String() instead of printf("%s", ...) ?

The backend interface doesn't have string. I could add String() (or just Name()) to the interface, or try to cast to fmt.Stringer here, but I thought the Sprintf was simpler. I don't have any real preference though...

it's fine as it is then.

I'd actually prefer to have a dedicated method in the interface. Otherwise you have to have implicit knowledge that stringifying a backend should give a short name of the backend, not e.g. full struct

sttts · 2018-03-06T08:31:25Z

staging/src/k8s.io/apiserver/pkg/server/options/audit.go

-		if err := validateBackendBatchConfig(pluginwebhook.PluginName, o.LogOptions.BatchOptions.BatchConfig); err != nil {
-			allErrors = append(allErrors, err)
+		if o.WebhookOptions.enabled() {
+			if err := validateBackendBatchOptions(pluginwebhook.PluginName, o.WebhookOptions.BatchOptions); err != nil {


the pattern we have everywhere else is to pass o.WebhookOptions to the validate func and check for nil (and ConfigFile) locally. This style complicates the code by making complexity non-local.

sttts · 2018-03-06T08:32:06Z

staging/src/k8s.io/apiserver/pkg/server/options/audit.go

-	if o.LogOptions.MaxSize < 0 {
-		allErrors = append(allErrors, fmt.Errorf("--audit-log-maxsize %v can't be a negative number", o.LogOptions.MaxSize))
+	// Check validities of MaxAge, MaxBackups and MaxSize of log options, if file log backend is enabled.
+	if o.LogOptions.enabled() {


the whole block belongs into validateLogOptions

crassirostris · 2018-03-06T14:09:03Z

As I've mentioned in #60719 (comment)

I think this PR should be merged before the freeze, the default parameters for the log backend should change.

I don't think it's possible currently to limit the number of goroutines running at the same time in the batching backend, so our option would be either to set the low flush timeout or low max batch size. The former is better, since the chance to have events ordered is higher and it allows to avoid write contention.

liggitt · 2018-03-06T16:45:27Z

/unassign
will let @tallclair weigh in on whether this is required for 1.10 now

tallclair · 2018-03-07T17:44:54Z

Discussed with @crassirostris, I think we should try and get this into 1.10. I'll address the comments in the next hour.

k8s-github-robot · 2018-03-07T18:06:06Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@crassirostris @sttts @tallclair

Pull Request Labels

sig/auth: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
kind/bug: Fixes a bug discovered during the current release.

Help

tallclair · 2018-03-07T18:29:45Z

Addressed feedback. I think the only unresolved issue is whether I should just add a "Name()" method to the backend interface.

ericchiang · 2018-03-07T20:10:35Z

Errors from golint:
staging/src/k8s.io/apiserver/plugin/pkg/audit/buffered/buffered.go:31:1: comment on exported const PluginName should be of the form "PluginName ..."

Please review the above warnings. You can test via "golint" and commit the result.
If the above warnings do not make sense, you can exempt this package from golint
checking by adding it to hack/.golint_failures (if your reviewer is okay with it).

sttts · 2018-03-08T07:46:03Z

staging/src/k8s.io/apiserver/pkg/server/options/audit.go

@@ -116,15 +116,15 @@ func NewAuditOptions() *AuditOptions {
 		WebhookOptions: AuditWebhookOptions{
 			BatchOptions: AuditBatchOptions{
 				Mode:        ModeBatch,
-				BatchConfig: defaultLogBatchConfig,
+				BatchConfig: pluginbuffered.NewDefaultBatchConfig(),


@crassirostris please confirm that these settings are what we want.

Yes, though it's preferrable to merge this PR after #60926, to avoid showing regressions in the scale tests

sttts · 2018-03-08T07:47:16Z

Looks good structurally. Please squash. @crassirostris ptal at the default config: #60739 (comment)

- Log backend defaults to blocking mode (backwards compatability) - Fix webhook validation - Add options test

sttts · 2018-03-09T12:23:12Z

/approve

Leaving final lgtm to @crassirostris

crassirostris · 2018-03-09T12:53:04Z

@tallclair Please unhold when #60926 is merged to avoid breaking scale tests

/lgtm
/hold

@tallclair

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Make log audit backend configurable in GCE This PR will allow to enable audit logging batching by default in e2e tests, after #60739 is merged. This is an important step to prevent a regression in scale tests. /cc @tallclair @sttts /assign @roberthbailey Robert, please approve ```release-note NONE ```

jdumars · 2018-03-13T14:41:14Z

@tallclair PTAL at test output

crassirostris · 2018-03-13T15:28:33Z

Unable to load build details

k8s-ci-robot · 2018-03-13T15:29:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crassirostris, sttts, tallclair

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/apiserver/OWNERS~~ [sttts]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ericchiang · 2018-03-13T16:23:36Z

#60926 is merged. @crassirostris can you remove your hold?

crassirostris · 2018-03-13T16:26:01Z

@ericchiang Thanks for pointing this out, done

k8s-github-robot · 2018-03-13T19:26:55Z

Automatic merge from submit-queue (batch tested with PRs 60737, 60739, 61080, 60968, 60951). If you want to cherry-pick this change to another branch, please follow the instructions here.

tallclair added kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. sig/auth Categorizes an issue or PR as relevant to SIG Auth. labels Mar 2, 2018

tallclair added this to the v1.10 milestone Mar 2, 2018

tallclair assigned sttts, liggitt and crassirostris Mar 2, 2018

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 2, 2018

k8s-github-robot added the milestone/needs-approval label Mar 2, 2018

k8s-ci-robot requested review from dims and ncdc March 2, 2018 23:25

liggitt mentioned this pull request Mar 5, 2018

fix advanced audit e2e test #60773

Closed

CaoShuFeng mentioned this pull request Mar 5, 2018

Audit use buffered backend #60237

Merged

CaoShuFeng reviewed Mar 5, 2018

View reviewed changes

crassirostris reviewed Mar 5, 2018

View reviewed changes

crassirostris added the area/audit label Mar 5, 2018

sttts reviewed Mar 6, 2018

View reviewed changes

crassirostris mentioned this pull request Mar 6, 2018

Advanced Audit tests flaking #60719

Closed

k8s-ci-robot unassigned liggitt Mar 6, 2018

tallclair added the status/approved-for-milestone label Mar 7, 2018

tallclair removed the milestone/needs-approval label Mar 7, 2018

tallclair force-pushed the audit-buffer branch from 4c6a9c9 to 8f506c2 Compare March 7, 2018 19:20

sttts reviewed Mar 8, 2018

View reviewed changes

crassirostris mentioned this pull request Mar 8, 2018

Make log audit backend configurable in GCE #60926

Merged

Fix default auditing options.

e004257

- Log backend defaults to blocking mode (backwards compatability) - Fix webhook validation - Add options test

tallclair force-pushed the audit-buffer branch from 9038c60 to e004257 Compare March 8, 2018 19:04

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2018

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 9, 2018

crassirostris closed this Mar 13, 2018

crassirostris reopened this Mar 13, 2018

crassirostris removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 13, 2018

k8s-github-robot merged commit c13d9ff into kubernetes:master Mar 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix default auditing options. #60739

Fix default auditing options. #60739

tallclair commented Mar 2, 2018

CaoShuFeng Mar 5, 2018

tallclair Mar 7, 2018

thockin commented Mar 5, 2018

crassirostris left a comment

crassirostris Mar 5, 2018

tallclair Mar 5, 2018

CaoShuFeng commented Mar 5, 2018

tallclair commented Mar 5, 2018

sttts Mar 6, 2018

tallclair Mar 7, 2018

sttts Mar 8, 2018

crassirostris Mar 8, 2018

sttts Mar 6, 2018

tallclair Mar 7, 2018

sttts Mar 6, 2018

tallclair Mar 7, 2018

crassirostris commented Mar 6, 2018

liggitt commented Mar 6, 2018

tallclair commented Mar 7, 2018

k8s-github-robot commented Mar 7, 2018

tallclair commented Mar 7, 2018

ericchiang commented Mar 7, 2018

sttts Mar 8, 2018

crassirostris Mar 8, 2018

sttts commented Mar 8, 2018

sttts commented Mar 9, 2018

crassirostris commented Mar 9, 2018

jdumars commented Mar 13, 2018

crassirostris commented Mar 13, 2018

k8s-ci-robot commented Mar 13, 2018

ericchiang commented Mar 13, 2018

crassirostris commented Mar 13, 2018

k8s-github-robot commented Mar 13, 2018

Fix default auditing options. #60739

Fix default auditing options. #60739

Conversation

tallclair commented Mar 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Mar 5, 2018

crassirostris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaoShuFeng commented Mar 5, 2018

tallclair commented Mar 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crassirostris commented Mar 6, 2018

liggitt commented Mar 6, 2018

tallclair commented Mar 7, 2018

k8s-github-robot commented Mar 7, 2018

tallclair commented Mar 7, 2018

ericchiang commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Mar 8, 2018

sttts commented Mar 9, 2018

crassirostris commented Mar 9, 2018

jdumars commented Mar 13, 2018

crassirostris commented Mar 13, 2018

k8s-ci-robot commented Mar 13, 2018

ericchiang commented Mar 13, 2018

crassirostris commented Mar 13, 2018

k8s-github-robot commented Mar 13, 2018